Skip to content
This repository was archived by the owner on Jul 16, 2023. It is now read-only.

feat: add 'include-methods' config to 'missing-test-assertion' rule #1026

Merged
merged 1 commit into from
Oct 10, 2022
Merged

feat: add 'include-methods' config to 'missing-test-assertion' rule #1026

merged 1 commit into from
Oct 10, 2022

Conversation

orevial
Copy link
Contributor

@orevial orevial commented Oct 9, 2022

What is the purpose of this pull request? (put an "X" next to an item)

  • Documentation update
  • Bug fix
  • New rule
  • Changes an existing rule
  • Add autofixing to a rule
  • Add a CLI option
  • Add something to the core
  • Other, please explain:

Completes issue #1019, PR #1023

What changes did you make? (Give an overview)

This PR aims at completing issue #1019 and PR #1023 about the new missing_test_assertion rule, by adding the possibility to enter additional test methods names to be checked in addition to the default test and testWidgets methods.

This new config might be useful when using dedicated test methods that potentially come from other packages, or just test wrappers written by the developers. E.g. we could create a base companyXTest() method that acts as a wrapper around test/testWidgets and sets some basic configuration for our company tests.

As a side note, I also changed the severity from "style" to "warning" in the doc, seems more appropriate.

Is there anything you'd like reviewers to focus on?

Config name, I can change it if "include-methods" is not clear enough.

@incendial incendial self-requested a review October 10, 2022 06:00
@incendial incendial added type: enhancement New feature or request area-rules labels Oct 10, 2022
@incendial
Copy link
Member

@orevial LGTM 🚀 , could you update the changelog?

@orevial orevial force-pushed the feat-missing-test-assertion-rule branch from dd7ba97 to 307e120 Compare October 10, 2022 07:34
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@codecov
Copy link

codecov bot commented Oct 10, 2022

Codecov Report

Merging #1026 (307e120) into master (5408c36) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1026      +/-   ##
==========================================
+ Coverage   86.85%   86.86%   +0.01%     
==========================================
  Files         327      327              
  Lines        7014     7021       +7     
==========================================
+ Hits         6092     6099       +7     
  Misses        922      922
Impacted Files Coverage Δ
...les_list/missing_test_assertion/config_parser.dart 100.00% <100.00%> (ø)
...ng_test_assertion/missing_test_assertion_rule.dart 100.00% <100.00%> (ø)
...les/rules_list/missing_test_assertion/visitor.dart 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@orevial
Copy link
Contributor Author

orevial commented Oct 10, 2022 •
edited
Loading

@incendial I'm trying to test the new rule on my project by importing the package from Git but for some reason the rule does not trigger anything, am I missing something ?

dart_code_metrics:
    git:
      url: https://github.com/orevial/dart-code-metrics.git
      ref: feat-missing-test-assertion-rule

My sample unit test :

test('trigger test lint', () {
    // do nothing, should trigger lint
});

@incendial
Copy link
Member

@incendial I'm trying to test the new rule on my project by importing the package from Git but for some reason the rule does not trigger anything, am I missing something ?

dart_code_metrics:
    git:
      url: https://github.com/orevial/dart-code-metrics.git
      ref: feat-missing-test-assertion-rule

My sample unit test :

test('trigger test lint', () {
    // do nothing, should trigger lint
});

Hm... have you added the rule to the analysis_options?

@orevial
Copy link
Contributor Author

orevial commented Oct 10, 2022

Hm... have you added the rule to the analysis_options?

🤦‍♂️

Thank you 😅

@orevial
Copy link
Contributor Author

orevial commented Oct 10, 2022

@incendial I'm trying to test the new rule on my project by importing the package from Git but for some reason the rule does not trigger anything, am I missing something ?

dart_code_metrics:
    git:
      url: https://github.com/orevial/dart-code-metrics.git
      ref: feat-missing-test-assertion-rule

My sample unit test :

test('trigger test lint', () {
    // do nothing, should trigger lint
});

Hm... have you added the rule to the analysis_options?

Actually even then it's still not using the rule with my incorrect tests...

@incendial
Copy link
Member

I'll merge it and take a look a little bit later, right now I don't see anything wrong with the rule

@incendial incendial merged commit 83432e7 into dart-code-checker:master Oct 10, 2022
@incendial
Copy link
Member

Screenshot 2022-10-10 at 18 47 27

Everything looks fine, just added the rule name to the analysis options and called dart pub run dart_code_metrics:metrics analyze test

@orevial orevial deleted the feat-missing-test-assertion-rule branch October 10, 2022 14:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@incendial incendial Awaiting requested review from incendial

Assignees
No one assigned
Labels
area-rules hacktoberfest-accepted type: enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@orevial @incendial